[WIP] Refactor #search in CustomersController#1
Open
mgbtukr wants to merge 2 commits intolysenko:mimemagic_freefrom
Open
[WIP] Refactor #search in CustomersController#1mgbtukr wants to merge 2 commits intolysenko:mimemagic_freefrom
mgbtukr wants to merge 2 commits intolysenko:mimemagic_freefrom
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DO NOT MERGE - Tests not working - WIP!
So after founding out, that #search method in CustomersController contains a lot of business logic, I decided to split it and move it to services.
In the main CustomerService class, I decided to write #perform method in high-level style, so the code will be more understandable.
I found out, that search by entity and search all entities using the same queries, so I moved it to one place and re-using it, so if we changed something, we will update only one place, and also it's much easier to de-bug if there is a bug somewhere.
I also note, that we have limit parameter, but we using it only in the view layer. I didn't do anything here, since I don't know the whole logic and how should it work, but If I do it from scratch or fully rewriting - I'll move limit to queries, so I'll already take from database limited records, because (it's really bad) if we have 1 million records - we will take all of them and only limit them on view layer.